improve BarStack to work with all scales type #368
Conversation
There was a problem hiding this comment.
First, thanks for this. Before landing this, a couple of architecture thoughts.
- Instead of checking scale type we should check feature support.
This would mean
function scaleHasBandwidth(scale) {
return isScaleBand(scale) || isScalePoint(scale)
}becomes
function scaleHasBandwidth(scale) {
return !!scale.bandwidth && typeof scale.bandwidth === 'function';
}My reason for this is in the future we may add more scales and if those scales implement bandwidth() they should be able to work with BarStack without us having to update the shape implementation.
- if we check on feature support instead of scale type then we can drop the
isScaleBand(), isScalePoint()util functions. This has the added benefit of don't have to add@vx/scaleas a dependency to@vx/shape(smaller bundles, faster install times).
Let me know what you think about the above approach. Happy to land this PR as is, I just had a lot of coffee this morning.
|
Yeah that's exactly the approach I initially considered.
Looking at it again though, I believe that, as you suggest, checking for feature support is probably better and more future proof if we add more scales implementing the same functions. Also, the smaller bundle is definitely a good point 😉 |
🚀 Enhancements
ScaleBandwas passed as scale prop; this enhance those components by checking for existence of specific interface functions and avoiding throwing errors with different scales.🐛 Bug Fix